Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[LW-9145] TxSummary refactor #883

Merged
merged 2 commits into from
Mar 27, 2024
Merged

[LW-9145] TxSummary refactor #883

merged 2 commits into from
Mar 27, 2024

Conversation

VanessaPC
Copy link
Contributor

@VanessaPC VanessaPC commented Feb 8, 2024 •
edited
Loading

Checklist

  • JIRA - LW-9145 LW-9710
  • Proper tests implemented
  • Screenshots added.

Proposed solution

  • Refactor the transaction summary for dapps
  • add to and from sections
  • txType and origin

Testing

  • Run the app
  • Go to wingriders and try to get some wingrider token from ada, there shouldn't be tokens
  • Do the reverse transactions, you should see the token
  • Go to handle.me , mint a token, you should see the transaction

Tx notes:

  • A transaction doesn't have to have to and from addresses. Sometimes it may have one sections with various, and sometimes both. But not always.
  • Transactions from swapping places such as Wingrider or SundaeSwap work with a batcher which means that you might see an amount in their interface, and a slightly different amount of ADA or tADA in the dapp, because you will see the amount of coins that don't get sent to a batcher to process the following transaction to actually create the swap.

Screenshots

Screenshot 2024-02-21 at 08 28 02
Screenshot 2024-02-21 at 08 29 17
Screenshot 2024-02-21 at 08 33 55

// This image is here only to show the display of to address / from address sections, the styles apply from the previous snapshots.
Screenshot 2024-02-14 at 08 35 05

@VanessaPC VanessaPC self-assigned this Feb 8, 2024
@VanessaPC VanessaPC requested review from a team as code owners February 8, 2024 12:46
@VanessaPC VanessaPC marked this pull request as draft February 8, 2024 12:46
@github-actions github-actions bot added browser Changes to the browser application. ui-toolkit labels Feb 8, 2024
@VanessaPC VanessaPC force-pushed the feat/lw-9145-tx-summary-refactor branch from fcd04cd to 428021f Compare February 9, 2024 20:17
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 9, 2024 •
edited
Loading

Allure report

allure-report-publisher generated test report!

smokeTests: ✅ test report for 9319be6d

passed failed skipped flaky total result
Total 30 0 0 0 30 ✅

@coveralls
Copy link

coveralls commented Feb 9, 2024 •
edited
Loading

Coverage Status

coverage: 50.567% (-0.9%) from 51.421%
when pulling c380794 on feat/lw-9145-tx-summary-refactor
into 007b6fe on main.

@VanessaPC VanessaPC marked this pull request as ready for review February 12, 2024 12:50
@VanessaPC VanessaPC changed the title Feat/lw 9145 tx summary refactor [LW-9145] TxSummary refactor Feb 12, 2024
Base automatically changed from LW-9094-new-wallet-manager to main February 13, 2024 17:07
@mchappell mchappell requested a review from a team as a code owner February 13, 2024 17:07
@DominikGuzei
Copy link
Member

@VanessaPC it seems like you need to update this PR to the latest main as the changed file count jumped from around 50 to over 200 and i can see many unrelated changes on Github 🤔

@github-actions github-actions bot added staking Changes to the staking package. e2e Changes to the e2e testing instrumentation. ci CI related issues or pull requests. and removed staking Changes to the staking package. e2e Changes to the e2e testing instrumentation. ci CI related issues or pull requests. labels Feb 14, 2024
pczeglik-iohk
pczeglik-iohk reviewed Feb 14, 2024
View reviewed changes
Copy link
Contributor

@pczeglik-iohk pczeglik-iohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the code and posted a few comments.

Nothing crucial, but there are a few improvements which can lead to increased consistency and maintenance of the code.

Let me know what do you think :)

and thank you @VanessaPC for great work.

packages/ui/src/design-system/dapp-transaction-summary/dapp-transaction-summary.css.ts Outdated Show resolved Hide resolved
packages/ui/src/design-system/dapp-transaction-summary/dapp-transaction-assets.component.tsx Outdated Show resolved Hide resolved
packages/icons/src/AdaComponent.tsx Outdated Show resolved Hide resolved
packages/core/src/ui/components/DappTransactionHeader/DappTransactionHeader.tsx Outdated Show resolved Hide resolved
packages/core/src/ui/components/DappTransaction/DappTransaction.tsx Outdated Show resolved Hide resolved
apps/browser-extension-wallet/src/hooks/useChainHistoryProvider.ts Outdated Show resolved Hide resolved
apps/browser-extension-wallet/src/features/dapp/components/ConfirmTransaction.tsx Outdated Show resolved Hide resolved
apps/browser-extension-wallet/src/features/dapp/components/ConfirmTransaction.tsx Outdated Show resolved Hide resolved
packages/ui/src/design-tokens/theme/hooks/use-theme-variant.tsx Show resolved Hide resolved
packages/ui/src/design-system/grid/grid.component.tsx Show resolved Hide resolved
@github-actions github-actions bot added the e2e Changes to the e2e testing instrumentation. label Feb 14, 2024
@VanessaPC VanessaPC force-pushed the feat/lw-9145-tx-summary-refactor branch from 202305f to 5885e71 Compare February 20, 2024 16:36
@VanessaPC
Copy link
Contributor Author

@DominikGuzei @pczeglik-iohk this is ready for re-review.

pczeglik-iohk
pczeglik-iohk approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@pczeglik-iohk pczeglik-iohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great. Thank you @VanessaPC

@VanessaPC
Copy link
Contributor Author

This PR introduces less code coverage on e2e tests, because it introduces also more functionality to test, I created LW-9857 to increase test coverage.

DominikGuzei
DominikGuzei approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great now 🎉 I just left one small suggestion that you can optionally look into (not really the scope of this PR)

@VanessaPC VanessaPC force-pushed the feat/lw-9145-tx-summary-refactor branch from 0e2b3ed to 7e7439d Compare March 4, 2024 19:30
@DominikGuzei DominikGuzei mentioned this pull request Mar 8, 2024
Merged
2 tasks
@VanessaPC VanessaPC force-pushed the feat/lw-9145-tx-summary-refactor branch 2 times, most recently from 3e41375 to 67669ab Compare March 22, 2024 12:06
@github-actions github-actions bot removed browser Changes to the browser application. e2e Changes to the e2e testing instrumentation. ui-toolkit labels Mar 25, 2024
@VanessaPC VanessaPC force-pushed the feat/lw-9145-tx-summary-refactor branch 2 times, most recently from 6139c0d to a32a78e Compare March 26, 2024 11:17
@VanessaPC VanessaPC enabled auto-merge (squash) March 26, 2024 11:46
@VanessaPC VanessaPC disabled auto-merge March 26, 2024 11:46
@VanessaPC VanessaPC force-pushed the feat/lw-9145-tx-summary-refactor branch from 43960c5 to 09ac2ff Compare March 26, 2024 14:24
pczeglik-iohk
pczeglik-iohk reviewed Mar 26, 2024
View reviewed changes
...tension-wallet/src/features/dapp/components/confirm-transaction/DappTransactionContainer.tsx
() => userRewardAccounts && userRewardAccounts.map((key) => key.address),
[userRewardAccounts]
);
const rewardAccountsAddresses = useMemo(() => userRewardAccounts?.map((key) => key.address), [userRewardAccounts]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @VanessaPC

@VanessaPC
feat: transaction representation refactor
9319be6 
- transaction includes transaction fees
- it displays returned deposit and deposits
- it shows from address sections wiht tokens and nfts
- it shows to address section with tokens and nfts
- it displays tokens and nfts in the summary
- it summarises the balance in coins of a transactions
- it displays the dapp name in the origin tab
@VanessaPC VanessaPC force-pushed the feat/lw-9145-tx-summary-refactor branch from 5ecea11 to 9319be6 Compare March 27, 2024 08:49
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 27, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@VanessaPC VanessaPC merged commit e8a9096 into main Mar 27, 2024
11 of 15 checks passed
@VanessaPC VanessaPC deleted the feat/lw-9145-tx-summary-refactor branch March 27, 2024 09:18
@pczeglik-iohk pczeglik-iohk mentioned this pull request Apr 5, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pczeglik-iohk pczeglik-iohk pczeglik-iohk approved these changes

@DominikGuzei DominikGuzei DominikGuzei approved these changes

@bslabiak bslabiak bslabiak approved these changes

@renanvalentin renanvalentin Awaiting requested review from renanvalentin

@lucas-barros lucas-barros Awaiting requested review from lucas-barros

@tomislavhoracek tomislavhoracek Awaiting requested review from tomislavhoracek

Assignees

@VanessaPC VanessaPC

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@VanessaPC @coveralls @DominikGuzei @bslabiak @pczeglik-iohk